Go Fish Harness - #1376

The worked example can be an illegal move once the game has more than 2 players.

The comment above this block claims the example is drawn from the hand "so the format line is never illegal advice", but the target and rank letter are chosen independently and the target is never checked for legality:

example_target = next((p.get("player") for p in players if p.get("player") != player_id), 1)

This takes the first non-self player regardless of whether they still hold cards. The engine's GenerateAsks (go_fish.cc:416-428) skips any target with PlayerCounts(target) == 0, so a card-less opponent is never a legal target. _format_other_players (line 205) already knows this and annotates such players (no cards -- cannot be asked) — so the example line can directly contradict the roster line printed a few lines above it.

Verified on GoFishGame({"players": 3}) across 7,539 Ask nodes: 63 (0.8%) rendered an example absent from legalActionStrings — e.g. example 0f when legals were ['1f','1m'], and example 0m when legals were ['1m']. At 2 players it never fires (0/4,400 nodes), which is why test_example_move_is_legal passes.

Suggested fix — prefer a target known to hold cards:

example_target = next(
    (p.get("player") for p in players
     if p.get("player") != player_id and p.get("cards", 0) > 0),
    next((p.get("player") for p in players if p.get("player") != player_id), 1),
)

Low urgency today: go_fish is registered in GAMES_LIST (open_spiel_env.py:1109) with no players parameter, so it always runs 2-player. But the engine supports up to 10 players and this harness otherwise generalizes cleanly over num_ranks/num_suits, so it is worth closing rather than relying on the registration string staying unchanged.

Two engine rules missing from the rules block, both under the production defaults ("go_fish" is registered bare in open_spiel_env.py:1117):

1. A miss with an empty pool does not draw -- your turn just ends (go_fish.cc:243-249: if (PoolSize() > 0) {...} else { advance = true; }). This text says unconditionally that a miss means you draw. Fires on 60/3,301 turns (~1/game), always in the endgame where turn economy decides the last books.
2. Emptying your hand is not elimination. CheckEmptyAsk (go_fish.cc:287-309) sends you to kEmptyDraw, you draw one card, and with ask_after_empty_draw=true you still get to ask. Happened in 60/60 games. A model that doesn't know this may play to avoid running its hand down -- the more consequential of the two.

(Also unstated: if hand and pool are both empty, play skips past you.)

Parser silently submits the wrong rank for 1J / 1K — collision window open on ~16.5% of ask turns.

The docstring names this as an "accepted trade-off" and harness_test.py:127-140 locks the behavior in, so I measured it before pushing back. The exposure is larger than "a genuine remaining edge" suggests.

The glyph set has exactly two collisions where an uppercase card label folds onto a different rank's action letter:

1Q → 1q is safe (no q letter exists, so it correctly falls through to rethink). 1A → 1a is safe (A's label and letter coincide). So the ambiguity is not hypothetical-but-rare; it is exactly the two face cards a model is most likely to name.

I replayed 300 random episodes (16,583 ask turns) and counted turns where the model's intended action and the folded action are both legal — i.e. where the substitution happens silently, with no rethink and no log:

model writes '<t>J' -> silently asks for 10:   2773/16583 turns (16.7%)
model writes '<t>K' -> silently asks for J:    2718/16583 turns (16.4%)

This is the worst shape of the ghost-fallback anti-pattern: the harness submits a move the model never chose, the model then sees that move in its own annotated move_history next turn (via the otherwise-excellent _annotate_move_history), and its rank-tracking deductions are poisoned from that point forward. It is strictly worse than a rethink, because a rethink is recoverable and this is invisible to both the model and the logs.

Two things weaken the trade-off argument as written. First, J, Q, K are the glyphs a card-game-pretrained model is most likely to reach for, and the prompt puts them on screen as rank names on every single turn — J: 1 (ask-letter 'k') in the hand lines, k=J, m=K in the legend. The docstring's defense ("the prompt only ever teaches action letters") is true but doesn't help: the prompt also displays J and K as labels right next to those letters. Second, the current design fails selectively — a model writing 1Q gets a helpful rethink, while the same model writing 1K gets silently misrouted. Failing consistently is better than failing selectively.

The fix does not require going fully case-sensitive (which, as the docstring correctly argues, would reject a model that merely uppercased the letter it was told to use). Reject only the ambiguous glyphs and let every other uppercase letter keep folding:

def _match_move_to_legal(raw, legal_action_strings, num_ranks=13):
    compact = _SEPARATORS_RE.sub("", raw.strip())
    labels = _label_to_letter(num_ranks)
    # Uppercase glyphs that are a card label for one rank AND the action
    # letter of a different rank (J, K on the standard deck). Case-folding
    # these picks a rank the model did not name, so defer to the rethink loop.
    ambiguous = {lab for lab, let in labels.items() if len(lab) == 1 and lab.lower() != let}
    if len(compact) >= 2 and compact[-1] in ambiguous:
        return None
    return _default_match(compact, legal_action_strings)

I ran this against current behavior:

'1a' current='1a'  fixed='1a'     '1J' current='1j'  fixed=None   <-- now rethinks
'1A' current='1a'  fixed='1a'     '1K' current='1k'  fixed=None   <-- now rethinks
'1M' current='1m'  fixed='1m'     '1L' current='1l'  fixed='1l'
'1I' current='1i'  fixed='1i'     '1, j' current='1j' fixed='1j'

All cosmetic-drift tolerance (separators, non-colliding uppercase) is preserved; only the two genuinely ambiguous glyphs now route to RETHINK_ILLEGAL, which already tells the model to re-read its ask-letters. raw_action is still populated by parse_json_action, so the rethink shows the model exactly what it wrote.

One wrinkle: parse_json_action calls the matcher with a (raw, legals) signature, so num_ranks needs a default or a closure. Defaulting to 13 is fine given the env registers plain "go_fish" (open_spiel_env.py:1109); a functools.partial from parse_response would be cleaner if ranks ever becomes configurable.

harness_test.py:127-140 and :146-149 will need updating to assert the rethink path rather than the substitution.

Prompt denies a game-end path the engine implements (missing-game-end-path anti-pattern).

The rules paragraph closes with:

The game ends when every card is in a book; the player with the most books wins.

That asserts a strict winner in all cases, but go_fish.cc:397-412 (Returns(), most_books_wins_ branch) has an explicit all-way-tie path:

int num_winners = std::count(player_books_.begin(), player_books_.end(), max_score);
if (num_winners == num_players_) {  // all way tie
  return std::vector<double>(num_players_, 0.0);
}

With the default 13 ranks a 2-player tie is arithmetically impossible (13 is odd), which is why I'm marking this warning rather than critical — it does not fire on the shipped config. But it is reachable the moment ranks is even. Measured at ranks=12, 2 players: 58/400 episodes (14.5%) ended 6–6 and returned [0.0, 0.0].

The proxy already models this correctly — go_fish_proxy.py:236-237 emits winner: "draw" on a tie — so the prompt is the only layer in the stack that denies it. That asymmetry is worth closing even at the default config, since a model that believes a draw is impossible will misvalue endgame positions where it is behind and cannot catch up.

Suggested rewrite of the final clause:

The game ends when every card is in a book; the player with the most books wins, and if every player finishes with the same number of books the game is a draw.

One sentence, correct under any ranks value, costs nothing on the default deck.

Rank legend degenerates to a tautology on non-13-rank decks.

_rank_label returns chr(ord("a") + rank_index) whenever num_ranks != 13, and _label_to_letter then maps that label to the identical letter. The legend renders as identity pairs. Actual output at ranks=12:

Rank letters: each rank is written as a letter in rank order -- a=a, b=b, c=c, d=d, e=e, f=f, g=g, h=h, i=i, j=j, k=k, l=l.

versus the useful 13-rank form (a=A, b=2, ..., k=J, l=Q, m=K).

This is prompt noise rather than a false claim — the hand lines still render correctly (c: 2 (ask-letter 'c')), so play isn't broken. But it burns tokens restating an identity and reads as a rendering bug to a model looking for signal.

Since _deck_params goes to real effort to support configurable decks (payload first, serialized-game fallback, sane defaults), worth completing that support:

if num_ranks == 13:
    rank_legend = ", ".join(f"{letter}={label}" for label, letter in label_to_letter.items())
else:
    rank_legend = f"the first {num_ranks} letters, a through {chr(ord('a') + num_ranks - 1)}, in rank order"

Low priority — the env registers plain "go_fish" at open_spiel_env.py:1109, so 13 ranks is what ships today.

Minor: _annotate_move_history deserializes twice and replays the whole game each turn. Verified correct — just noting the redundancy.

Lines 314-316 call pyspiel.deserialize_game_and_state(serialized) twice, using only game from the first and only final_state from the second. One call gives you both:

game, final_state = pyspiel.deserialize_game_and_state(serialized)
history = final_state.history()
replay = game.new_initial_state()

I want to be explicit that I checked this function's correctness rather than eyeballing it, since it is the most intricate logic in the file and the place a subtle error would be hardest to spot. I instrumented a full game and compared every annotation against the engine's true per-ask outcome, cross-checking the received formula against player_cards()[target][rank] read directly off the state before each ask:

total asks 57, own asks p0=29 p1=28  -- 0 mismatches, 0 formula errors

The num_suits book correction is right, the mover == player_id gate correctly skips chance nodes (draws and the deal), and the move_history/outcomes alignment held throughout. This recovers information the engine genuinely withholds — go_fish.cc:536-540 walks events_ backward and stops at the observer's own last action, so a player never sees their own ask results. Good catch by the author; it's the highest-value part of this harness.

On cost: the replay is O(history) per turn and grows with the game. Worst case measured was 3.2ms at 28 own-asks, ~0.10s total across a 57-ask game — negligible against an LLM call. Not worth optimizing; just drop the duplicate deserialize.

A hit does not always grant another turn. The prompt states this unconditionally, but CheckEmptyAsk (go_fish.cc:296-307) runs after every ask: if your hit takes the last cards off the only other player still holding any, there is nobody left to ask, so the engine sets kEmptyDraw and calls AdvancePlayer -- the turn passes to someone else.

seed 7: p1 asked p0 for rank 12 (a hit), emptying them.
  after: phase=EmptyDraw current_player=0  -> asker retained turn? False

Measured over 600 self-play games: 38/11,506 hits (0.33%), occurring in 6.2% of games. Rare per-move, but it fires in one game in sixteen, and when it does the model has been told the opposite of what happens.

This is a different branch from the CheckEmptyAsk case in the open thread above -- that one is PlayerCounts(current_player_) == 0 (you have no cards); this is the !askee path (nobody else has cards). Suggested one-clause fix:

they give you all of them and you take another turn (ask again) -- unless that leaves every other player with no cards, in which case play passes on.

Positional zip inverts every annotation if move_history is a suffix rather than the full history. The guard handles len(move_history) > len(outcomes) (extras left bare), but the shorter case misaligns silently -- move_history[0] gets paired with outcomes[0], the outcome of the observer's first ask of the game, not of that move.

RESTARTED agent, move_history=['1e','1g'] (its last two asks):
  1: 1g (received 1)      should read: 1g (go fish)

Every entry is attributed to the wrong ask, which is worse than showing nothing: the model builds its opponent model on inverted data and there is no signal that anything is off. Reachable if the agent process restarts mid-episode (create_agent_fn's move_history closure starts empty while the engine state is mid-game), or if a future caller windows the history for prompt-length reasons.

The arithmetic in this function is correct -- I verified 0 mismatches across 2,101 prompt-time renders, matching the other reviewers -- it's specifically the alignment assumption that is unguarded. Anchoring from the end and bailing on an incompatible shape fixes it:

if len(move_history) > len(outcomes):
    return None  # can't establish alignment; fall back to the bare history
offset = len(outcomes) - len(move_history)
annotated = [
    f"{mv} ({outcomes[offset + i]})" if outcomes[offset + i] else mv
    for i, mv in enumerate(move_history)
]

wanted is 93% redundant with known_has on the same line. Follow-on to the resolved staleness thread, not a reopen -- the two filters added in _deductions (booked-rank retirement, known_void expiry) correctly fix what was raised there. What remains is the overlap: 46,351 of 49,770 emitted wanted entries (93.1%) name a rank already listed in known_has for that same player.

  Player 1: known to hold 4>=1, 7>=2, 9>=3, J>=2, Q>=2; has asked for A, 4, 5, 7, 9, J, Q

Only A and 5 carry information the rest of the line doesn't already state more precisely -- known_has gives a count, wanted just repeats the rank. Filtering wanted against the ranks in known_has (either here or in the proxy) surfaces the ~7% that adds something. Legibility rather than correctness, but this is the densest line in the prompt and it's mostly noise.